Repository navigation
Conversation
…ti-entry builds The completion task assumed output_files was laid out [js, map, js, map, ...] and linked each JS output's .sourcemap to the next array element. OutputFileList actually lays them out as [chunks..., sourcemaps/bytecode..., additional...], so with 2+ entrypoints or splitting, a.js.sourcemap would be b.js and the last chunk got the first chunk's map. Each OutputFile already carries the absolute source_map_index into the array. Resolve .sourcemap from that index instead of next-element chaining.
|
No actionable comments were generated in the recent review. WalkthroughThe bundle completion task now tracks sourcemap positions through output compaction and associates each ChangesSourcemap association refactor
Suggested reviewers: Severity of issue fixed: Medium Merge Risk:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/api/js_bundle_completion_task.rs`:
- Around line 669-681: The source map lookup in
js_bundle_completion_task::do_compilation is using stale source_map_index values
after output_files is swap-compacted, which can cause chunks to attach the wrong
.map artifact when splitting is enabled. Update the compaction logic to
recompute and store new source_map_index values for kept entries, or thread an
old-to-new index map through the compaction and lookup path so the loop that
calls get_index on output_files_js always uses post-compaction indexes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3daec19-4065-4164-b947-5113e69904b5
📒 Files selected for processing (2)
src/runtime/api/js_bundle_completion_task.rstest/bundler/bun-build-api.test.ts
When compile is set, do_compilation keeps only the entry point and written sourcemaps and swap-compacts the array, but left each kept entry's source_map_index pointing at its pre-compaction position. With splitting that resolved the entry's .sourcemap to another chunk's map. Record an old-to-new index map during compaction and rewrite source_map_index on survivors; dropped targets remap to u32::MAX (no link). Adds .sourcemap assertions to the existing compile+external and compile+splitting tests to guard this path.
|
Updated 5:02 PM PT - Nov 10th, 2026
✅ @robobun, your commit 89c65229ee993942154f728c77824efc103cd44e passed in 🧪 To try this PR locally: bunx bun-pr 33588That installs a local version of the PR into your bun-33588 --bun |
There was a problem hiding this comment.
LGTM — the compile+splitting remap fix addresses my earlier concern and the added tests cover the multi-entry, splitting, and compile paths.
Extended reasoning...
Overview
This PR fixes BuildArtifact.sourcemap returning the wrong artifact when Bun.build() produces more than one JS output. The old code assumed [js, map, js, map, ...] interleaving and assigned the next array element as the sourcemap; the actual layout is [chunks..., sourcemaps..., additional...]. The fix drops the next-element chaining in on_complete_anytask in favor of a two-pass approach: first materialize all BuildArtifact wrappers into output_files_js, then wire each chunk's .sourcemap by looking up output_files_js[source_map_index]. A follow-up commit (0d29725) adds an old_to_new index remap in do_compilation's swap-compact so source_map_index stays valid after compile-mode drops non-entry chunks — this was the regression I flagged in my earlier review, and it's fixed exactly as suggested.
Test additions: 4 parameterized cases in bun-build-api.test.ts (two entrypoints × {splitting on/off} × {linked, external}) asserting each non-sourcemap output's .sourcemap is the kind: "sourcemap" artifact at <path>.map; plus .sourcemap assertions added to the existing compile+external and compile+splitting+external tests in bun-build-compile-sourcemap.test.ts, the latter verifying the linked map's sources includes entry.js (catches wrong-chunk assignment).
Security risks
None. This is purely output-metadata wiring on the JS-thread completion path — no parsing of untrusted input, no auth/crypto/permissions, no new FFI or unsafe blocks (the one unsafe is the pre-existing BuildArtifact.sourcemap.set pattern, unchanged).
Level of scrutiny
Moderate. This is a logic change in the Bun.build() JS-API completion path, not a config tweak, and it interacts with the compile-mode compaction that already produced one subtle bug during review. That said: the mechanism is simple (use the index the bundler already stores instead of positional adjacency), the one interaction I identified was fixed correctly, the old_to_new remap is bounds-safe (old_to_new.len() = pre-compact output_files.len(), which bounds every valid source_map_index), and dropped targets correctly remap to u32::MAX so the second pass skips them.
Other factors
- No CODEOWNERS coverage for these paths.
- The bug-hunting system found no issues on the current revision.
- My earlier review's only finding is resolved; the only commit since the fix is a CI retrigger.
- Existing single-entry
.sourcemaptests (BuildArtifact properties sourcemap,sourcemap: true with outdir) continue to exercise the unchanged happy path.
|
Status: the fix is pushed (8b911b4). It waits for CI and for a review. Reproduction on a debug build of // a.ts and b.ts hold one console.log() line each
const r = await Bun.build({ entrypoints: ["./a.ts", "./b.ts"], sourcemap: "linked" });
for (const o of r.outputs) console.log(o.path, "->", o.sourcemap?.path ?? null);
// ./a.js -> ./b.js expected: ./a.js.map
// ./b.js -> ./a.js.map expected: ./b.js.map
// ./a.js.map -> null
// ./b.js.map -> nullWith this PR the script prints |
|
Issue #37184 reports another trigger for this same defect: a single entrypoint that imports CSS with sourcemap: "linked" produces outputs [js, css, js.map], and the next-element pairing assigns the CSS asset as the JS artifact's .sourcemap. Confirmed this PR's index-based pairing fixes that case too. This branch has conflicts with main now. I rebased the fix onto current main (the artifact.sourcemap.set call was removed on main since the cached value now lives in the wrapper's m_sourcemap slot, so only sourcemap_set_cached is needed) and added a test for the single-entry CSS trigger, verified fail-before on system bun and pass-after on the debug build: https://github.com/oven-sh/bun/tree/farm/3da9e510/sourcemap-pairing-css-asset |
|
Ran into the same bug independently (another session handed it off as a separate report); this PR is the same fix, so I am not opening a second one. Two notes for the rebase, since the branch currently conflicts with
|
The two conflicts are in src/runtime/api/js_bundle_completion_task.rs and test/bundler/bun-build-api.test.ts. This merge takes main for every file. The next commit applies the fix to main's code.
`on_complete` stored the next element of `outputs` as the `.sourcemap` of each output that has a source map. The output list is `[chunks, source maps and bytecode, additional files]`, so the next element is the map only when the build has one chunk. - `on_complete` walks the list from the last file to the first and stores the wrapper of the file at `source_map_index`. It stores only the wrapper of a file of kind `sourcemap`. - `do_compilation` sets the `source_map_index` of the entry point to the position of its map after the compaction, or to `u32::MAX` when the map is not kept. - The standalone HTML filter asserts that no kept file names another file by its position. - `BuildArtifact.sourcemap` is declared as `BuildArtifact | null` again: native code stores only the wrapper of a source map.
|
This PR has a new diff as of 8b911b4. It replaces the version from July, which conflicted with
The description has the measurements and the list of tests. |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
The compile + splitting stale-index concern from my earlier review is addressed by the entry_at/entry_source_map_at remap in do_compilation. Beyond the inline findings, I also checked the reverse walk in on_complete for sparse-array or ordering problems (the array is preallocated with create_empty_array(len) and the list builder always places a map after its chunk, so get_direct_index sees the wrapper), and the standalone HTML filter in generateChunksInParallel.rs (only HTML and Sourcemap files survive, neither of which carries an index, so the release build needs no remap there). I did not run the test suite or the BUN_JSC_validateExceptionChecks=1 check named in the description.
Extended reasoning...
The change rewires how BuildArtifact.sourcemap is assigned in on_complete (lookup by source_map_index, walked last to first) and remaps the executable's indices after do_compilation compacts the output list, plus two debug asserts, a bindgen return-type narrowing and new tests; no security-sensitive surface. Two confirmed findings are posted inline (a debug-only panic path for compile aot and a certified-silent sourcemap write failure), so a human still needs to weigh those; the note above records what else was examined and ruled out this run.
The other files of the list are gone once the executable is written, so the kept file names none of them by position.
There was a problem hiding this comment.
I re-reviewed the latest push and found no new bugs; the new take_executable_file in src/runtime/cli/compile_command/build.rs clears the three position fields on the file finish_compilation keeps, which covers the compile: { aot: true } path I flagged earlier. Because this changes the artifact graph that every Bun.build caller observes (and the invariant that source_map_index survives every list compaction), a human look is still worthwhile.
What was reviewed:
- The reverse loop in
on_complete:output_files_jsis pre-sized bycreate_empty_array, so filling it last-to-first and reading a later index withgetDirectIndexis sound (a hole reads back as a non-cell and is skipped);resultis only held across non-allocating calls beforeput_index. - The
do_compilationswap-compact: the entry always precedes its map in the list, soentry_at < entry_source_map_at, and the fallback tou32::MAXwhen the map write fails matches the new unwritable-map test. packages/bun-types/bun.d.tsalready declaressourcemap: BuildArtifact | null, so the narrowed.bind.tstype needs no .d.ts change.- Not done here: running the new tests against the unfixed build to confirm they fail for the right reason (the PR description claims 24 fail on main; I did not execute them).
Extended reasoning...
The change rewires how BuildArtifact.sourcemap is assigned in src/runtime/api/js_bundle_completion_task.rs (reverse iteration keyed on source_map_index instead of next-element chaining), remaps the entry's indices after the compile-path compaction, clears positions on the CLI compile product in src/runtime/cli/compile_command/build.rs, adds two debug_asserts, narrows a bindgen return type, and adds a parameterized test matrix plus compile tests. It touches no security-sensitive surface. The latest commit addresses the aot-path assert concern raised in the prior run, and no third-party objection is outstanding (coderabbitai threads were resolved by a non-author). It is not approved outright because the fix spans three list-compaction sites in the bundler with release-build behavior that silently yields null when an index is stale, and the fail-on-unfixed-build claim was not verified by running tests.
Fixes #37184
Problem
BuildArtifact.sourcemapis the next element ofoutputs, not the output's map. With two entry points,a.js.sourcemapisb.js. With a CSS import, it is the CSS asset.console.logof an output prints each later chunk under it, through.sourcemap.on_complete(src/runtime/api/js_bundle_completion_task.rs) stored the next wrapper as.sourcemap. The list order is[chunks, maps and bytecode, additional files].Fix
on_completewalks the list last to first and stores the wrapper atsource_map_index. A map comes after its file, and every other reader uses the field as a position.do_compilationremoves files from the list and now gives the executable its map's new position.test/bundler/bun-build-api.test.tsand 2 inbun-build-compile-sourcemap.test.tsfail on main. Alsostandalone.test.ts,bundler_html.test.ts.Background
OutputFileis one file of a build.source_map_indexis the position of its map in the same list, oru32::MAX.BuildArtifactis the JS wrapper of anOutputFile. Onlyon_completefills.sourcemap.compilebuild.Downsides
getDirectIndexcall (0 to 1). Allocations do not change. The release binary grows by 256 bytes..sourcemapand the printed text of an output change for builds with two or more chunks. A stored snapshot of that text must be regenerated.Notes
Measurements (linux-x64. Debug build for counts, release
bun-profilefor sizes. Mainaf566a38e5against the same commit with this diff.).sourcemappairings wrong: 45 of 53 map-owning outputs on main, 0 of 53 with this diff. 33 configurations: one entry, two entries, two entries + splitting, a JS entry that imports CSS, an HTML entry, bytecode with two entries,dtswith two entries (each withlinkedandexternal, with and withoutoutdir), thencompile,compile+ splitting,compile+ a CSS import,compilewith[index.html, x.ts]and with[x.ts, index.html].on_completeon the JS thread (gdb breakpoint hit counts, three runs each, all identical), asgetDirectIndex / getIndex / setInternalField / putIndex:0/0/0/1to0/0/0/10/0/1/2to1/0/1/20/0/2/4to2/0/2/4compile+ splitting + external maps:0/0/1/3to1/0/1/3on_complete(__rust_alloc/__rust_deallochit counts, same runs):5/1,8/2,14/4and72/73, before and after. The diff adds no loop overoutput_files.llvm-size).on_complete: 19,197 to 19,470 bytes (llvm-nm --print-size).generate_chunks_in_paralleldoes not change (39,904 bytes).BuildArtifactcells while one output of an 8-entry build with external maps is held: 9 to 2 (heapStats()afterBun.gc(true)). On main each output kept the next output alive through.sourcemap. ProtectedBuildArtifactcount: 0 to 0../eN.jsfrom the working directory,sourcemap: "external":Bun.inspect(outputs[0])is 49,621 characters with 64 artifacts under it on main, and 364 characters with 1 artifact with this diff.Bun.inspect(outputs)is 1,269,191 to 31,940 characters. A build with one entry prints the same text before and after.Other changes in the diff
compilewith entry points[index.html, app.js]was wrong on main: the executable's.sourcemapwas the map of the page chunk. The othercompileshapes were correct on main only because the executable's map was the next element. The existing tests forcompileand forcompile+ splitting now assert the pairing..sourcemapisnull. On main it was the map of another chunk. The new test puts a directory at the path of the map. The build reportssuccess: trueand prints the write error to stderr, as on main.do_compilationalso clearsbytecode_indexandmodule_info_indexon the executable, because the same compaction removes those files. OnlyStandaloneModuleGraph.rsreads these two fields, before the compaction.generateChunksInParallel.rskeeps HTML files and maps. Adebug_assert!states that none of them names another file by position, so there is nothing to remap. Failure path, checked once by hand: with the filter changed to keep every file, a standalone page build with external maps panics with "a kept file names another file by its position before the filter".on_completehas adebug_assert!that asource_map_indexnames a later file of kindsourcemap. Failure path, checked once by hand: with the remap indo_compilationdisabled,compile+ a CSS import panics with "output file 0 names 2, which is not a later Sourcemap file". A release build that finds a position that is not a map stores nothing, and.sourcemapisnull.BuildArtifact::write_format) follows.sourcemapwith no depth limit. It stops after one level because a map has no.sourcemap. The kind check inon_completekeeps that true.compile+ splitting test fail.JSBundler.bind.tsdeclaressourcemapasBuildArtifact | nullagain. AOT: front end, test runs and tests for the rewritten compiler #47942 addedundefinedbecause the old code could store theundefinedthatto_jsgives for aNoopfile. The new code stores only the wrapper of a map.OutputFile.rsgets one doc line on each of the three position fields.compile: { aot: true }keeps one file, the main entry point's, as the executable.take_executable_fileincompile_command/build.rsnow clears its three positions when it truncates the list, so thedebug_assert!inon_completeholds on this path too..sourcemapof that output isnullbefore and after.Not changed
output_files[source_map_index]directly (HTMLBundle.rs,StandaloneModuleGraph.rs,bake/production/mod.rs) are correct today.referenced_css_chunksalso holds positions and is stale afterdo_compilation. Nothing reads it after that point.source_map_indexstays a plainu32position. Each place that removes files remaps it by hand (typed_files.rs,aot/assets.rs,aot/page_bundles.rs,aot/output.rs, and nowdo_compilation). A shared helper for all of them was considered. It changes code that is correct today, and it adds an allocation and two passes to eachcompilebuild.History
do_compilation. It conflicted with main after BuildArtifact: drop redundant Strong for .sourcemap; use the wrapper's WriteBarrier slot #34668 removed the Rust-sidesourcemapfield and One termination signal, one fold: Err(Thrown) always means an exception is pending, and each event-loop dispatcher takes it in one place #37275 changed thepromise.rejectshape. This version keeps its two sites and its two-entry tests, and replaces the mechanism. The reviews and the two review threads of 2026-07-07 are about the first version.Test runs on this machine (linux-x64, debug + ASAN)
bun-build-api.test.tsalso pass withBUN_JSC_validateExceptionChecks=1.standalone.test.ts(26),bundler_html.test.ts(29),bun-build-api-dts.test.ts(8) andbundler_compile_splitting.test.ts(30) pass.bun-build-api.test.ts: 114 pass. "can be called thousands of times" (180 s limit) times out with and without this diff.bun-build-compile.test.ts: 74 pass, 7 time out in the full run. Three--bytecodetests also time out alone, with and without this diff. The other four pass alone.compiletests copy a 1.6 GB debug binary each. On this machine some of the older tests inbun-build-compile-sourcemap.test.tspass the 5 s limit in one run and not in the next, with and without this diff.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bundler/bun-build-compile-sourcemap.test.ts, test/bundler/bun-build-api.test.ts